Ignore a connect timeout superseded by a newer attempt on the same peripheral - #1676
Ignore a connect timeout superseded by a newer attempt on the same peripheral#1676krishrathi1 wants to merge 4 commits into
Conversation
…ripheral beginCentralConnection's timeout closure only checked isConnecting && !isConnected before tearing down a link -- it never checked whether the attempt it was scheduled for was still the current one. A disconnect followed by a reconnect while the original timeout is still armed (ordinary BLE usage: walking behind an obstacle, a pocket, a crowd) starts a fresh attempt via beginConnecting, which is still mid-flight and healthy when the stale timer from the earlier attempt fires. That timer finds isConnecting == true (true for the new attempt) and cancels the connection out from under it, then applies recordConnectionTimeout's scoring penalty and discovery-ignore cooldown to a peer that never actually timed out -- degrading mesh connectivity for a healthy, in-range peer, repeatably, with nothing a user would recognize as a bug. Captures the Date passed to beginConnecting for this specific attempt and compares it against the link state's lastConnectionAttempt inside the timeout closure; beginConnecting (and armPendingBackgroundConnects, which sets it nil) both overwrite that field on every new attempt for the same peripheralID, so a mismatch means a newer attempt has since started and this timer's own attempt is no longer the live one. No Xcode/Swift toolchain available here, verified by tracing the state machine rather than compiling. No automated regression test: this class has no existing test coverage (grepped bitchatTests -- only its BLEConnectionScheduler dependency is unit-tested in isolation, not BLERadioController's own connect/timeout logic), and CoreBluetooth's CBCentralManager/CBPeripheral have no public initializers, so a from-scratch mock harness would itself be unverified without a compiler. The fix is 12 lines added around one guard clause and reviews by inspection; flagging the coverage gap rather than shipping an unverified test alongside an unverified fix.
Chessing234
left a comment
Chessing234
left a comment
There was a problem hiding this comment.
traced this through and the mechanism holds — beginConnecting stores the date verbatim (BLELinkStateStore.swift:114) so the == round-trips exactly, and the nil case from armPendingBackgroundConnects correctly reads as "superseded" rather than matching.
two things i'd raise anyway.
using Date as the attempt identity works but couples correctness to timestamps being distinct. a monotonically increasing attempt counter on the link state would say what it means, and can't be confused by a clock adjustment mid-attempt — NTP stepping the clock backwards between beginConnecting and the timeout would make a live attempt look superseded and leak the timer.
second, there's no test. this is precisely the class of bug that comes back, because reproducing it needs a disconnect and reconnect inside the timeout window and nobody does that by hand twice. the timeout closure is hard to test as written, but the decision — given a captured attempt stamp and the current state, should this timeout act — is a pure function and would be worth extracting for one.
Chessing234
left a comment
Chessing234
left a comment
There was a problem hiding this comment.
capturing attemptStartedAt and comparing it to lastConnectionAttempt is the right staleness check — the old isConnecting guard alone couldn't tell a superseded reconnect from the original attempt. looks good.
|
Updated the PR to address review feedback:
|
|
Fixed parameter signature in BLEConnectTimeoutPolicyTests.swift (initializeMemory count argument). CI re-triggered. |
…inate CBPeripheral test mock crash
|
Refactored BLEConnectTimeoutPolicy.shouldExecuteConnectTimeout to accept pure primitive parameters (capturedAttemptToken, isConnecting, isConnected, currentAttemptToken, isPeripheralConnected). This allows unit testing the policy without instantiating fake CBPeripheral mock pointers, eliminating ARC memory management crashes in iOS runner. |
2 participants
The bug
BLERadioController.beginCentralConnection's timeout closure (bitchat/Services/BLE/BLERadioController.swift) is scheduledbleConnectTimeoutSeconds(8s) after every connect attempt. When it fires, its only staleness check is:It never checks whether this is still the attempt it was scheduled for.
Failure scenario
Ordinary BLE usage: a peer connects, then walks behind an obstacle / into a pocket / through a crowd and disconnects a few seconds later -- well within the 8s timeout window.
didDisconnectPeripheraltears the old link state down andtryConnectFromQueue()immediately starts a new attempt, callingbeginConnectingagain (a freshlastConnectionAttempt). That new attempt is healthy and mid-flight when the original attempt's timer fires. The guard above only checksisConnecting && !isConnected-- true for the new attempt too -- so the stale timer cancels the new, live connection, tears it down, and callsscheduler.recordConnectionTimeout, applying a discovery-ignore cooldown and a weak-link scoring penalty to a peer that never actually timed out.Result: a healthy, in-range peer's legitimate reconnection gets cancelled and then artificially deprioritized -- repeatable indefinitely in any marginal-signal environment (pocket, crowd, subway), silently degrading mesh connectivity with nothing a user would recognize as a bug (no crash, no error visible anywhere).
The fix
Captures the
Datepassed tobeginConnectingfor this specific attempt (attemptStartedAt) and compares it againststate.lastConnectionAttemptinside the timeout closure.beginConnectingoverwrites that field on every new attempt for the sameperipheralID(so doesarmPendingBackgroundConnects, which sets itnilfor background wake-on-proximity connects, a deliberately different mechanism with no timeout closure of its own). A mismatch means a newer attempt has started since this timer was scheduled, so the stale timer now just logs and returns instead of tearing anything down.Verification
No Xcode/Swift toolchain available here, so this is verified by tracing the state machine, not compiling:
beginConnectingfully replaces the peripheral's link-state entry (not a partial update) on every call, so any secondbeginConnectingfor the sameperipheralIDnecessarily changeslastConnectionAttempt.isConnecting: true(armPendingBackgroundConnects) explicitly setslastConnectionAttempt: nilon purpose (existing comment: "an indefinite pending connect has no attempt clock") and schedules no timeout closure of its own -- so it can never collide with or accidentally satisfy this check.cancelStalePendingConnects()(the foreground-return sweep) is an independent mechanism keyed onlastConnectionAttemptage directly, unaffected by this change.No automated regression test. Grepped
bitchatTests--BLERadioControllerhas zero existing test coverage; only itsBLEConnectionSchedulerdependency is unit-tested in isolation.CBCentralManager/CBPeripheralhave no public initializers, so a from-scratch CoreBluetooth mock harness would itself be unverified without a compiler to check it against. Given that, I'd rather ship the 21-line fix (reviewable by inspection, the diff is small) and flag the coverage gap honestly than add unverified test infrastructure alongside an unverified fix. Happy to build test scaffolding for this class as a follow-up if that's wanted, or to have someone with the toolchain confirm behavior directly.